-
Notifications
You must be signed in to change notification settings - Fork 100
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: don't use lexical sorting for version numbers in codegen #1168
base: main
Are you sure you want to change the base?
fix: don't use lexical sorting for version numbers in codegen #1168
Conversation
Previously, python-libjuju iterated over schemas keyed by their version string (e.g. '3.1.9') using lexical sorting. For a given facade version, a definition in a higher versioned schema was intended to overwrite any prior definition saved (see generate_facades function in facade.py). With lexical sorting, '3.1.10' would be sorted in between '3.1.1' and '3.1.2', which would not lead to the desired behaviour. This commit fixes this problem by using a tuple of integers as the sorting key. A special case is requried for the version string 'latest', and we use (9000, 9000, 9000).
337d749
to
1392759
Compare
if version_string == 'latest': | ||
return (9000, 9000, 9000) | ||
# raise ValueError if string isn't in the format A.B.C | ||
major, minor, micro = version_string.split('.') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wdyt about using what already do for the client and server version:
from packaging import version
version.parse(server_version)
juju/client/connector.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like a good idea, if you want to take care of it to push this over the finish line that would be cool, otherwise I'll hopefully get to it some time during the sprint
captures = defaultdict(codegen.Capture) | ||
|
||
# Build the Facade classes | ||
for juju_version in sorted(schemas.keys()): | ||
for juju_version in sorted(schemas.keys(), key=sortable_schema_version): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the idea here to go from oldest to newest because the latter iteration overwrites the ealier?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Email reply isn't as cool as I was hoping. But yeah, I think it does overwrite.
The relevant line is:
captures[schema.version].clear(cls_name)
With this PR I'm just slavishly trying to keep the original intent working. But maybe this being technically broken is an opportunity for us to make some bigger changes?
Certainly in terms of implementation it would be nice to get rid of all the inheriting from dict
, and have those classes just do operations on an internal dict
instead -- at most implement the specific dict
methods that are expected to be used and forward them to the internal dict
rather than inheriting every dict
method and overriding some of them ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh and note that schema.version
is a facade version, like 7
.
juju_version
is the Juju schema version, like '3.1.10'
.
…ema-release-process #1169 #### Description We want to ensure that `python-libjuju` supports the latest Juju schemas, and that it's clear to `python-libjuju` maintainers and users what versions are supported. This PR adds documentation on how to do this (`docs/CONTRIBUTING.md`), and follows this process for the existing schemas, updating the `3.1` series (`juju/client/SCHEMAS.md` and `juju/client/schemas-juju-*.json`) #### QA Steps No changes to generated code are introduced in this PR. `make client` should not produce any changes to the repo state. Test should all pass. #### Notes & Discussion Replaces: - #1166 Implicitly depends on: - #1168 Since we add a schema for `3.1.10` here.
Description
Previously, python-libjuju iterated over schemas keyed by their version string (e.g.
'3.1.9'
) using lexical sorting. For a given facade version, a definition in a higher versioned schema was intended to overwrite any prior definition saved (seegenerate_facades
function infacade.py
). With lexical sorting,'3.1.10'
would be sorted in between'3.1.1'
and'3.1.2'
, which would not lead to the desired behaviour. This commit fixes this problem by using a tuple of integers as the sorting key. A special case is required for the version string'latest'
, and we use(9000, 9000, 9000)
.QA Steps
Codegen doesn't change with current schemas.
Notes & Discussion
When
Juju 9001
comes out (well, any Juju version >9000.9000.9000
), we'll need to update the special case for'latest'
. Actually we can probably just remove the'latest'
special case if we ever tidy up codegen, as it's not used currently, and is probably just intended as a convenience for testing and development, where it still doesn't even really seem necessary.